feat: add lazy CLI extensions - #863
Merged
nabinchha merged 3 commits intoAug 18, 2026
Merged
Conversation
andreatnvidia
marked this pull request as ready for review
August 13, 2026 16:42
Contributor
Greptile SummaryAdds metadata-only discovery and lazy loading for optional top-level CLI extensions.
|
| Filename | Overview |
|---|---|
| packages/data-designer/src/data_designer/cli/lazy_group.py | Adds metadata-only entry-point discovery, deferred extension loading, compatibility validation, and deterministic collision handling; no eligible follow-up finding was established. |
| packages/data-designer/src/data_designer/cli/main.py | Enables optional extension discovery on the root CLI group. |
| packages/data-designer-slurm/src/data_designer/slurm/cli.py | Adds the initial empty Slurm command group and its Click-command factory. |
| packages/data-designer-slurm/pyproject.toml | Registers the Slurm command factory in the new CLI entry-point group. |
| packages/data-designer/tests/cli/test_lazy_group.py | Covers lazy discovery, dispatch, compatibility checks, collisions, malformed registrations, and nested-command behavior. |
| scripts/test_slurm_package_install.py | Extends wheel smoke testing with extension visibility, lazy-import assertions, and root-help startup budgets. |
Sequence Diagram
sequenceDiagram
participant User
participant Root as data-designer root
participant Metadata as Entry-point metadata
participant Factory as Extension factory
participant Command as Extension command
User->>Root: --help
Root->>Metadata: Discover names and summaries
Metadata-->>Root: Extension metadata
Root-->>User: Help without importing extension
User->>Root: slurm ...
Root->>Metadata: Resolve selected entry point
Root->>Root: Validate version compatibility
Root->>Factory: Load and invoke factory
Factory-->>Root: click.Command
Root->>Command: Dispatch arguments
Command-->>User: Result
Reviews (3): Last reviewed commit: "refactor: order public CLI members first" | Re-trigger Greptile
andreatnvidia
changed the base branch from
andreatnvidia/feat/slurm-package
to
feat/slurm-execution
August 17, 2026 14:27
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
nabinchha
force-pushed
the
andreatnvidia/feat/lazy-cli-extensions
branch
from
August 18, 2026 16:00
d549d81 to
d657a4c
Compare
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
nabinchha
approved these changes
Aug 18, 2026
nabinchha
left a comment
nabinchha
left a comment
Contributor
There was a problem hiding this comment.
Thanks for putting this together, @andreatnvidia!
Summary
This adds metadata-only discovery for optional top-level CLI groups, validates and loads only the selected extension, and registers the optional Slurm package as the first consumer. The implementation matches the PR's stated lazy-loading and failure-isolation goals.
Findings
Suggestions — Take it or leave it
packages/data-designer-slurm/src/data_designer/slurm/cli.py:9 — Keep completion controls at the root
- What: The Slurm
typer.Typeruses the defaultadd_completion=True, sodata-designer slurm --helpexposes--install-completionand--show-completion. The root app explicitly disables these options, and built-in subgroups do not expose them. - Why: This makes the first extension group behave differently from the rest of the CLI and lets a nested group re-enable shell-completion controls that the root intentionally omits.
- Suggestion: Set
add_completion=Falseon the Slurm app and add a small help-output assertion to keep the embedded group aligned with the root CLI.
What Looks Good
- Root help remains metadata-only; the wheel smoke test verifies that neither the Slurm module nor compatibility-checking modules are imported.
- Compatibility failures, malformed registrations, duplicate names, and built-in collisions are isolated with targeted errors and strong behavioral coverage.
- The generic nested-command test verifies real dispatch behavior and preserves leaf exit codes rather than only checking mocks.
Verdict
Ship it (with nits) — the completion-option inconsistency is optional and does not block merge.
This review was generated by an AI assistant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a minimal entry-point seam for optional packages to provide top-level
data-designercommand groups without importing their implementation until selected.Related issues
Closes #853
Depends on #856
Related to #850
Changes
data_designer.clientry-point metadata.Impact
A base-only install has no
slurmcommand. Installingdata-designer[slurm]addsdata-designer slurm, while root help and unrelated commands do not importdata_designer.slurmor compatibility-checking modules.F2 intentionally establishes the extension seam and an empty Slurm group. Leaf subcommands are owned by follow-up API work. A generic nested-command test proves the seam supports deeper command trees and preserves leaf exit codes.
This PR is stacked on #856 and should be retargeted to
mainafter F1 merges.Testing
.venv/bin/pytest packages/data-designer/tests packages/data-designer-slurm/tests -p no:cacheprovider(1146 passed, 1 skipped)make test-slurm-wheel-install(0.574 s base median, 0.568 s extension median; 1 s base and 100 ms overhead budgets).venv/bin/ruff check --fix ..venv/bin/ruff format .